Skip to content

BLD: Try using shared memory utilities in Cython to reduce wheel sizes #61384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lithomas1
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@lithomas1 lithomas1 added the Build Library building on various platforms label May 1, 2025
@lithomas1 lithomas1 added this to the 2.3 milestone May 1, 2025
@lithomas1
Copy link
Member Author

lithomas1 commented May 1, 2025

around 10% savings in both compressed/uncompressed size
(1MB compressed, 4MB uncompressed)

scikit-learn seems to have gotten a lot more mileage out of this optimization...
(they report around 25% savings)

@lithomas1 lithomas1 marked this pull request as ready for review May 1, 2025 00:49
@@ -47,6 +47,24 @@ endif
cy = meson.get_compiler('cython')
if cy.version().version_compare('>=3.1.0')
add_project_arguments('-Xfreethreading_compatible=true', language: 'cython')

# Use shared utility code to reduce wheel sizes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool seems useful. Is there any discussion to add this natively to Meson instead of custom-coding this ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how that's possible, since it generates an extra extension module and projects may have their code set up (e.g., with a Meson generator) to apply Cython/C compile args to all their extension modules. They may also have opinions about where the extension module should live in the import tree.

@@ -5,7 +5,7 @@ requires = [
"meson-python>=0.13.1",
"meson>=1.2.1,<2",
"wheel",
"Cython~=3.0.5", # Note: sync with setup.py, environment.yml and asv.conf.json
"Cython==3.1.0rc1", # Note: sync with setup.py, environment.yml and asv.conf.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to do this, no need to force it since 3.1.0 is brand new (users and distros may have reasons not to upgrade, and there may be bugs).

You can use this instead:

if cy.version().version_compare('>=3.1.0')
  cython_args += ['-Xfreethreading_compatible=True']

The way I did it in scipy/scipy#22796 is a bit more concise than in this PR, also with cython_args: passing in a single place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you do have the cy.version().version_compare('>=3.1.0') already, so just no need to pin Cython. You'll get 3.1.0 anyway by default now.

@rgommers
Copy link
Contributor

This looks pretty good as is, and will definitely be worthwhile to do - it's a major gain in binary size for only 20 lines of code or so. It should work now, maybe retrigger CI after dropping the Cython pin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants